Fix Oracle durability operation batch execution - #3615

@pedroandrade03 This isn't going to make the release today, but will by EOD Monday

Thanks

Review notes

Read this one closely since it touches the shared RDBMS durability path. The root cause is right — DatabaseOperationBatch concatenates every operation's SQL into one command with @ markers and reads multiple result sets back, and ODP.NET supports neither — and the shape of the fix (per-operation command, one Oracle transaction, opt-in hook so no other provider changes behavior) is the right instinct. The internal interface also sidesteps the default-interface-member trap we've hit before with mocks, and it compiles cross-assembly because Wolverine.RDBMS/AssemblyAttributes.cs:12 already has InternalsVisibleTo("Wolverine.Oracle").

A few things to work through.

1. Test coverage is the main gap

The reported failure in #3614 was ORA-00933/ORA-00936 at runtime, and the three new tests only exercise the three string helpers in isolation. Nothing in the suite would catch a regression in the actual batch execution path — the thing that was broken.

The good news is this is cheap: CIOracle already stands up gvenzl/oracle-free (build/CITargets.cs:321-331) and runs the whole OracleTests project against it in CI. A single integration test that persists an incoming/outgoing envelope, drives the real operation set through DatabaseOperationBatch.ExecuteAsync (or DatabaseBatcher), and asserts recovery would have caught the original bug and would guard the statement-splitting and marker-rewriting logic going forward. That's what I'd most like to see added before merge.

2. Parity divergences from the generic path

Worth deciding on each deliberately rather than by omission:

• IExceptionTransform — ApplyCallbacksAsync gives operations a chance to transform driver exceptions (DatabaseOperationBatch.cs:118,143). The Oracle path calls operation.ReadResultsAsync directly and skips that entirely. No IDatabaseOperation implements IExceptionTransform today, so nothing is lost right now, but it's a silent divergence that will quietly not work when someone adds one.
• ObjectDisposedException — the generic path swallows it and still returns postProcessingCommands(); the new branch returns AgentCommands.Empty. Arguably the new behavior is more correct on shutdown, but the two paths should agree.
• The dropped exceptions list in executeOperationAsync mirrors the generic path, which also collects and discards it. Parity preserved — no action, just noting I checked.

3. The string-level SQL surgery is the fragile part

splitStatements splits on every ; and normalizeParameterMarkers rewrites every @ followed by a letter or underscore — including occurrences inside string literals. I walked all of Wolverine.RDBMS/Durability/* plus Transport/PollDatabaseControlQueue and Transport/DeleteExpiredMessages, and nothing currently trips either one (the only inline literal is '{EnvelopeStatus.Incoming}' in MoveReplayableErrorMessagesToIncomingOperation). But nothing prevents it, and the failure mode would be a mangled statement at runtime, Oracle-only.

Minimum: state the assumption in a comment on both helpers. Better long-term option worth considering — the operations hardcode the marker themselves (MoveReplayableErrorMessagesToIncomingOperation.cs:28,32 writes @replayable into the SQL text), so having the shared operations emit a provider-supplied prefix would remove the need for any rewrite. That's a bigger change than this PR should carry, but it's the version that doesn't need a regex.

Related: the "returns data and must contain exactly one SQL statement" InvalidOperationException is currently unreachable — the only data-returning operations (CheckRecoverableIncoming/OutgoingMessagesOperation, DeleteExpiredDeadLetterMessagesOperation, PollDatabaseControlQueue) are all single-statement. Keeping the guard is fine, but it's a runtime-only trap for whoever writes the next multi-statement data-returning operation; a comment naming the constraint next to the throw would help them.

4. Naming and placement

• Per CLAUDE.md, casing follows accessibility: internal and public members are PascalCase, private/protected are camelCase. So normalizeParameterMarkers, splitStatements, and normalizeParameters are internal static and should be NormalizeParameterMarkers / SplitStatements / NormalizeParameters. executeOperationAsync (private static) is correct as-is.
• These three are pure SQL/parameter translation with no message-store state. Wolverine.Oracle/Util/ (alongside OracleCommandExtensions) is a more natural home than internal static members hanging off OracleMessageStore, and it would let the transport/listener code reuse them.
• normalizeParameters's Guid → OracleDbType.Raw + ToByteArray() conversion already exists in two other places (Util/OracleCommandExtensions.cs:27,53 and Sagas/OracleSagaSchema.cs:180). Worth consolidating rather than adding a third copy.

5. One question

normalizeParameters handles bool and Guid. DeleteExpiredDeadLetterMessagesOperation and DeleteExpiredEnvelopesOperation pass a DateTimeOffset through builder.AppendParameter, which then relies on ODP.NET's implicit mapping — while the rest of the store is explicit about OracleDbType.TimeStampTZ (e.g. OracleMessageStore.Incoming.cs:30). Did the downstream recovery scenario you ran exercise an expiry/keep_until comparison? If not, worth confirming that mapping before trusting it, since a silent timezone/precision mismatch there would show up as expired rows never being cleaned rather than as an error.

CI

Green across the full matrix on this branch, CIOracle included. (Minor: the description says it's opened as a draft, but the PR isn't marked draft.)

🤖 Review assisted by Claude Code

Follow-up to my review notes above, consolidating what needs to happen here so this doesn't have to be reconstructed later.

Where this stands. The diagnosis is correct and the shape of the fix is right — per-operation command, one Oracle transaction, opt-in hook so no other provider changes behavior. CI is green including CIOracle. What's holding it is not the design, it's that the change puts string-level SQL rewriting on the shared durability path with no test exercising that path.

The one thing I'd most like before merge: an integration test in OracleTests that drives the real durability operations through DatabaseOperationBatch.ExecuteAsync (or DatabaseBatcher) against Oracle and asserts recovery. CIOracle already stands up gvenzl/oracle-free and runs the whole project against it (build/CITargets.cs:321-331), so this costs a test file, not infrastructure. The three tests here cover NormalizeParameterMarkers / SplitStatements / NormalizeParameters in isolation; none of them would catch a regression in the thing that was actually broken in #3614.

The rest, in priority order:

1. splitStatements splits on every ; and normalizeParameterMarkers rewrites every @ followed by a letter or underscore — including inside string literals. I walked all of Wolverine.RDBMS/Durability/* plus Transport/PollDatabaseControlQueue and Transport/DeleteExpiredMessages and nothing currently trips either, but nothing prevents it either, and the failure would be a mangled statement at runtime on Oracle only. At minimum state the assumption in a comment on both helpers.
2. Parity gaps with the generic path: IExceptionTransform handling (DatabaseOperationBatch.cs:118,143) isn't replicated — no live loss today since no IDatabaseOperation implements it, but it will quietly not work when one does. And ObjectDisposedException returns AgentCommands.Empty here where the generic path still returns postProcessingCommands(); pick one deliberately.
3. Casing: per CLAUDE.md, casing follows accessibility — internal and public members are PascalCase, private and protected are camelCase. normalizeParameterMarkers, splitStatements, and normalizeParameters are internal static and should be NormalizeParameterMarkers / SplitStatements / NormalizeParameters. executeOperationAsync is private static and is correct as written.
4. Placement: these three are pure SQL/parameter translation with no message-store state. Wolverine.Oracle/Util/ (next to OracleCommandExtensions) is a better home than internal statics on OracleMessageStore, and the Guid → OracleDbType.Raw + ToByteArray() conversion in normalizeParameters already exists twice (Util/OracleCommandExtensions.cs:27,53 and Sagas/OracleSagaSchema.cs:180) — worth consolidating rather than adding a third copy.
5. The "returns data and must contain exactly one SQL statement" throw is currently unreachable (the only data-returning operations are all single-statement). Fine to keep, but a comment naming the constraint would help whoever writes the next multi-statement data-returning operation, since it's a runtime-only trap on Oracle.
6. Still open from before: DeleteExpiredDeadLetterMessagesOperation and DeleteExpiredEnvelopesOperation pass a DateTimeOffset through builder.AppendParameter and rely on ODP.NET's implicit mapping, while the rest of the store is explicit about OracleDbType.TimeStampTZ (e.g. OracleMessageStore.Incoming.cs:30). Did your downstream recovery run exercise an expiry / keep_until comparison? A mismatch there would show up as expired rows never being cleaned rather than as an error, so it's worth confirming rather than assuming.

Worth considering, separately from this PR. The rewrite exists because the shared operations hardcode the marker into the SQL text (MoveReplayableErrorMessagesToIncomingOperation.cs:28,32 writes @replayable). Having those operations emit a provider-supplied prefix would remove the need for any regex at all. That's a bigger change than this PR should carry, and it's a maintainer call whether it's the direction — but it's the version that doesn't need string surgery.

@pedroandrade03 — thanks for the thorough write-up on the original PR, and for tracking this down past #3589. The IDatabaseOperationBatchExecutor hook was a good instinct: keeping it an internal interface rather than a default interface member also sidesteps a mocking trap we've hit before in this codebase.

🤖 Review assisted by Claude Code

Thanks for the thorough root-cause work here, @pedroandrade03 — the diagnosis in this PR is what made the fix possible, and your test cases are carried forward in #3659 with you as co-author.

We went a different route on the implementation. Rather than give Oracle a provider-specific IDatabaseOperationBatchExecutor and repair the already-compiled SQL, #3659 teaches the shared batching mechanics about statement boundaries and lets each provider decide what a boundary means. That's paired with JasperFx/weasel#390 (released as Weasel 9.19.0), which adds Weasel.Oracle.OracleDbCommandBuilder.

Two of the three things this PR fixed turned out to already exist in Weasel and just weren't wired up: the @ markers came from OracleMessageStore.ToCommandBuilder() handing back Weasel.Core.DbCommandBuilder, whose constructor hardcodes '@', and the Guid→RAW conversion was already in Weasel.Oracle.CommandBuilder. The reason neither was reachable is that Weasel.Oracle.CommandBuilder is a sibling of DbCommandBuilder rather than a subclass. The third thing — that ODP.NET genuinely cannot execute several statements from one command — is real and unavoidable, and your PR was right about that.

Working through it surfaced two cases the ;-splitting couldn't have covered:

• Four operations write more than one statement each (both ReleaseOrphanedMessages*, MoveReplayableErrorMessagesToIncoming, and PersistNodeRecord's insert-per-event), so splitting per operation isn't enough.
• :replayable is bound by two different statements, and AddNamedParameter finds-or-adds, so it exists exactly once — any index-based slicing binds it to one command and the other fails at execution.

Your PR also indirectly turned up a latent Weasel bug: the Guid→RAW conversion was a new member rather than an override, so every typed AppendParameter overload routed straight past it. Fixed in weasel#390 too.

Closing this in favour of #3659. Really appreciate the contribution.